Force commit messages output to UTF-8 - #331087
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Ladislau Szomoru (@lszomoru)Matched files:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves handling of non-UTF-8 commit message encodings by forcing Git log output to UTF-8 and adds a smoke test to validate that behavior.
Changes:
- Added a smoke test that commits an EUC-JP encoded message and asserts it’s read back as UTF-8.
- Updated Git process spawning to apply
i18n.logOutputEncoding=UTF-8via-cfor spawned Git commands.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| extensions/git/src/test/smoke.test.ts | Adds coverage for reading non-UTF-8 commit messages via repository log. |
| extensions/git/src/git.ts | Forces Git log output encoding to UTF-8 by injecting -c i18n.logOutputEncoding=UTF-8. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // commit messages may appear garbled, | ||
| // so the output encoding for commit messages is forced to UTF-8. | ||
| const spawnArgs = ['-c', 'i18n.logOutputEncoding=UTF-8', ...args]; | ||
|
|
||
| return cp.spawn(this.path, spawnArgs, options); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is still not resolved, and adding these args to every command may not be the best idea...
There was a problem hiding this comment.
Before I go into the details, please allow me to apologize for the long message.
This is based on machine translation, so I hope it reads clearly on your side.
As you pointed out, -c is currently added to all git invocations. This was a decision I made at the time of implementation.
I considered limiting the flag only to commands that reference i18n.logOutputEncoding, but I was concerned that future additions or changes to those commands might cause some cases to slip through, which could allow the encoding issue to reappear.
I also looked into identifying commands based on args[0]. However, with an implementation pattern like the one used in this change—where options such as -c may be inserted at the beginning of the argument list—there is a possibility that args[0] will no longer represent the command name. If similar changes are introduced in the future, command detection based on args[0] could easily break. For that reason, I felt that relying on args[0] would not be structurally safe.
I also evaluated parsing options or inspecting the entire args array to determine the command type, but performing a complete and reliable analysis would require significant implementation effort, and at the time I judged it to be impractical.
Commands that do not use this setting (such as status or push) simply ignore it, and I have not observed any side effects. Since spawn() already sets LANG, LC_ALL, and GIT_PAGER regardless of the command type, this approach did not seem to diverge significantly from the existing behavior.
That said, I fully recognize that adding -c to all invocations is not necessarily the optimal long‑term solution.
Now that I’m revisiting this behavior, I would very much appreciate any thoughts or suggestions you might have.
At the time, I chose this implementation mainly to reduce the risk of the issue recurring, but I’m open to improving it if there’s a better approach.
|
yutotnh (@yutotnh) Could you please address Copilot's comments? |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Dmitriy Vasyura (@dmitrivMS) Thanks for the comment. |
Fixes #331086
Summary
I modified the code to enforce the
i18n.logOutputEncoding=UTF-8setting when calling Git.Test
The test I added checks whether commit messages written in an encoding other than UTF-8 (EUC-JP) are retrieved as UTF-8.
Without my fix, they would be retrieved as EUC-JP, causing the test to fail.
Image
Additional Information
Even after applying this setting, my local test results showed that the execution time remained unchanged.
I conducted the following experiment in the vscode repository.
If no settings are specified
If a setting is specified